Skip to content

Conversation

nielsdos
Copy link
Member

If the first string conversion fails, then i==0, but memory was still allocated for params. However, we skip freeing params when i==0.

If the first string conversion fails, then i==0, but memory was still
allocated for `params`. However, we skip freeing `params` when i==0.
}
efree(params);
}
efree(params);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was it the "point of contention" ? in that case you can just change it place.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what you mean

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if num_params == 0 then params array was never freed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, e.g. if you exit here in the first iteration when i==0, then params is never freed:

php-src/ext/pgsql/pgsql.c

Lines 1130 to 1131 in 28ce1b0

_php_pgsql_free_params(params, i);
RETURN_THROWS();

Removing the num_params > 0 check fixes it. It is a useless check anyway: the loop will already check this and we should not do the check for freeing params.

Copy link
Member

@devnexen devnexen Oct 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure what I meant originally was just putting the efree(params); out of the if block would have worked as well but that s detail.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. That works too but since I'm touching the function anyway I thought I'd just remove the check

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I just also saw your other PR, makes sense

Copy link
Member

@devnexen devnexen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anyhow as long the fix is correct...

@nielsdos nielsdos closed this in cf3b70d Oct 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants